Skip to content

Conversation

RalfJung
Copy link
Member

@RalfJung RalfJung commented May 2, 2020

In #67531, we removed the rustc_promotable attribute from a bunch of Duration methods, but not from Duration::from_secs. This makes the current list of promotable functions the following (courtesy of @ecstatic-morse):

  • INT::min_value, INT::max_value
  • std::mem::size_of, std::mem::align_of
  • RangeInclusive::new (backing x..=y)
  • std::ptr::null, std::ptr::null_mut
  • RawWaker::new, RawWakerVTable::new ???
  • Duration::from_secs

I feel like the last one stands out a bit here -- the rest are all very core language primitives, and RawWaker has a strong motivation for getting a 'static vtable. But a &'static Duration? That seems unlikely. So I propose we no longer promote calls to Duration::from_secs, which is what this PR does.

#67531 saw zero regressions and I am not aware of anyone complaining that this broke their (non-cratered) code, so I consider it likely the same will be true here, but of course we'd do a crater run.

See this document for some more background on promotion and rust-lang/const-eval#19 for some of the concerns around promoting function calls.

@rust-highfive
Copy link
Contributor

r? @nikomatsakis

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 2, 2020
@nikomatsakis
Copy link
Contributor

@bors try

@bors
Copy link
Collaborator

bors commented May 2, 2020

⌛ Trying commit 58ae4a9 with merge b2a885c600d942fb829a214e798f7ae205a0594e...

@bors
Copy link
Collaborator

bors commented May 2, 2020

☀️ Try build successful - checks-azure
Build commit: b2a885c600d942fb829a214e798f7ae205a0594e (b2a885c600d942fb829a214e798f7ae205a0594e)

@RalfJung
Copy link
Member Author

RalfJung commented May 4, 2020

@nikomatsakis so do you agree this is worthwhile and we should try to crater it?

@nikomatsakis
Copy link
Contributor

I do, which is why I did the bors try command.

@craterbot check

@craterbot
Copy link
Collaborator

👌 Experiment pr-71796 created and queued.
🤖 Automatically detected try build b2a885c600d942fb829a214e798f7ae205a0594e
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 4, 2020
@crlf0710 crlf0710 added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels May 15, 2020
@craterbot
Copy link
Collaborator

🚧 Experiment pr-71796 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@pietroalbini
Copy link
Member

@craterbot p=1

@craterbot
Copy link
Collaborator

🚨 Error: it's only possible to edit queued experiments

🆘 If you have any trouble with Crater please ping @rust-lang/infra!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot
Copy link
Collaborator

🎉 Experiment pr-71796 is completed!
📊 2 regressed and 1 fixed (102844 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the blacklist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels May 21, 2020
@RalfJung
Copy link
Member Author

Both regressions are spurious. So, looks like on the part of the ecosystem that crater covers, this will not break anything.

@nikomatsakis what is the next step here?

@Mark-Simulacrum
Copy link
Member

With my release team hat on I'd say the zero regressions from the crater do look quite promising. I would suggest that we land this at the start of a nightly cycle, though, so ~June 4th -- in a few weeks -- just to give it as much time to bake as possible.

@nikomatsakis
Copy link
Contributor

nikomatsakis commented May 21, 2020

r=me but waiting until the start of the cycle seems fine.

@rust-lang/lang -- I'm not going to bother nominating this as I don't anticipate controversy. The proposal here is to remove Duration::from_secs from the list of functions that can be implicitly promoted into a constant. Crater tests found no regressions.

@joshtriplett
Copy link
Member

But a &'static Duration? That seems unlikely.

Would this still allow const { Duration::from_secs(1) } once that syntax exists? Does this just prevent automatically promoting a constant Duration?

@RalfJung
Copy link
Member Author

@joshtriplett yes and yes.

@joshtriplett
Copy link
Member

No objections then.

@nikomatsakis nikomatsakis added S-blocked Status: Blocked on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 22, 2020
RalfJung added a commit to RalfJung/rust that referenced this pull request Jun 6, 2020
de-promote Duration::from_secs

In rust-lang#67531, we removed the `rustc_promotable` attribute from a bunch of `Duration` methods, but not from `Duration::from_secs`. This makes the current list of promotable functions the following (courtesy of @ecstatic-morse):

* `INT::min_value`, `INT::max_value`
* `std::mem::size_of`, `std::mem::align_of`
* `RangeInclusive::new` (backing `x..=y`)
* `std::ptr::null`, `std::ptr::null_mut`
* `RawWaker::new`, `RawWakerVTable::new` ???
* `Duration::from_secs`

I feel like the last one stands out a bit here -- the rest are all very core language primitives, and `RawWaker` has a strong motivation for getting a `'static` vtable. But a `&'static Duration`? That seems unlikely. So I propose we no longer promote calls to `Duration::from_secs`, which is what this PR does.

rust-lang#67531 saw zero regressions and I am not aware of anyone complaining that this broke their (non-cratered) code, so I consider it likely the same will be true here, but of course we'd do a crater run.

See [this document](https://github.com/rust-lang/const-eval/blob/master/promotion.md) for some more background on promotion and rust-lang/const-eval#19 for some of the concerns around promoting function calls.
RalfJung added a commit to RalfJung/rust that referenced this pull request Jun 6, 2020
de-promote Duration::from_secs

In rust-lang#67531, we removed the `rustc_promotable` attribute from a bunch of `Duration` methods, but not from `Duration::from_secs`. This makes the current list of promotable functions the following (courtesy of @ecstatic-morse):

* `INT::min_value`, `INT::max_value`
* `std::mem::size_of`, `std::mem::align_of`
* `RangeInclusive::new` (backing `x..=y`)
* `std::ptr::null`, `std::ptr::null_mut`
* `RawWaker::new`, `RawWakerVTable::new` ???
* `Duration::from_secs`

I feel like the last one stands out a bit here -- the rest are all very core language primitives, and `RawWaker` has a strong motivation for getting a `'static` vtable. But a `&'static Duration`? That seems unlikely. So I propose we no longer promote calls to `Duration::from_secs`, which is what this PR does.

rust-lang#67531 saw zero regressions and I am not aware of anyone complaining that this broke their (non-cratered) code, so I consider it likely the same will be true here, but of course we'd do a crater run.

See [this document](https://github.com/rust-lang/const-eval/blob/master/promotion.md) for some more background on promotion and rust-lang/const-eval#19 for some of the concerns around promoting function calls.
@bors
Copy link
Collaborator

bors commented Jun 6, 2020

⌛ Testing commit 58ae4a9 with merge 23ff1657a24783bddd42e51cfa70902060439a9d...

RalfJung added a commit to RalfJung/rust that referenced this pull request Jun 6, 2020
de-promote Duration::from_secs

In rust-lang#67531, we removed the `rustc_promotable` attribute from a bunch of `Duration` methods, but not from `Duration::from_secs`. This makes the current list of promotable functions the following (courtesy of @ecstatic-morse):

* `INT::min_value`, `INT::max_value`
* `std::mem::size_of`, `std::mem::align_of`
* `RangeInclusive::new` (backing `x..=y`)
* `std::ptr::null`, `std::ptr::null_mut`
* `RawWaker::new`, `RawWakerVTable::new` ???
* `Duration::from_secs`

I feel like the last one stands out a bit here -- the rest are all very core language primitives, and `RawWaker` has a strong motivation for getting a `'static` vtable. But a `&'static Duration`? That seems unlikely. So I propose we no longer promote calls to `Duration::from_secs`, which is what this PR does.

rust-lang#67531 saw zero regressions and I am not aware of anyone complaining that this broke their (non-cratered) code, so I consider it likely the same will be true here, but of course we'd do a crater run.

See [this document](https://github.com/rust-lang/const-eval/blob/master/promotion.md) for some more background on promotion and rust-lang/const-eval#19 for some of the concerns around promoting function calls.
@RalfJung
Copy link
Member Author

RalfJung commented Jun 6, 2020

yield
@bors retry

@bors
Copy link
Collaborator

bors commented Jun 6, 2020

⌛ Testing commit 58ae4a9 with merge 6543d83271ab6a1d8c1553a1ddb183e604a96ebd...

RalfJung added a commit to RalfJung/rust that referenced this pull request Jun 6, 2020
de-promote Duration::from_secs

In rust-lang#67531, we removed the `rustc_promotable` attribute from a bunch of `Duration` methods, but not from `Duration::from_secs`. This makes the current list of promotable functions the following (courtesy of @ecstatic-morse):

* `INT::min_value`, `INT::max_value`
* `std::mem::size_of`, `std::mem::align_of`
* `RangeInclusive::new` (backing `x..=y`)
* `std::ptr::null`, `std::ptr::null_mut`
* `RawWaker::new`, `RawWakerVTable::new` ???
* `Duration::from_secs`

I feel like the last one stands out a bit here -- the rest are all very core language primitives, and `RawWaker` has a strong motivation for getting a `'static` vtable. But a `&'static Duration`? That seems unlikely. So I propose we no longer promote calls to `Duration::from_secs`, which is what this PR does.

rust-lang#67531 saw zero regressions and I am not aware of anyone complaining that this broke their (non-cratered) code, so I consider it likely the same will be true here, but of course we'd do a crater run.

See [this document](https://github.com/rust-lang/const-eval/blob/master/promotion.md) for some more background on promotion and rust-lang/const-eval#19 for some of the concerns around promoting function calls.
@RalfJung
Copy link
Member Author

RalfJung commented Jun 6, 2020

@bors retry

@bors
Copy link
Collaborator

bors commented Jun 6, 2020

⌛ Testing commit 58ae4a9 with merge 42cda8ef3fabb5418ef717b08c7f056188ad4738...

@RalfJung
Copy link
Member Author

RalfJung commented Jun 6, 2020

@bors retry rollup=always

bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 6, 2020
Rollup of 3 pull requests

Successful merges:

 - rust-lang#71796 (de-promote Duration::from_secs)
 - rust-lang#72508 (Make `PolyTraitRef::self_ty` return `Binder<Ty>`)
 - rust-lang#72708 (linker: Add a linker rerun hack for gcc versions not supporting -static-pie)

Failed merges:

r? @ghost
@bors bors merged commit 64c27f9 into rust-lang:master Jun 7, 2020
@flip1995
Copy link
Member

flip1995 commented Jun 7, 2020

@Mark-Simulacrum this was merged, even though it breaks Clippys UI tests: https://dev.azure.com/rust-lang/rust/_build/results?buildId=31276&view=logs&j=e50ab380-190b-535c-8a18-25c988f7577b&t=4a9c8583-1de4-50df-1657-629e71b187fc&l=7899

Admittedly one Clippy UI test is broken in the rustc testsuite anyway (something, something, proc_macro), but I thought with #72674 this shouldn't happen anymore.

flip1995 added a commit to flip1995/rust-clippy that referenced this pull request Jun 7, 2020
@Mark-Simulacrum
Copy link
Member

Hm yeah I would've expected that to not happen to. I'll try to take a look tomorrow.

@RalfJung RalfJung deleted the from-secs branch June 7, 2020 08:00
@Mark-Simulacrum
Copy link
Member

Fixed in #73097

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants